Overview

This PR addresses critical command injection vulnerabilities identified by CodeQL code scanning alerts. The vulnerabilities existed in two locations where user-controllable input was concatenated into shell command strings without proper sanitization.

Security Issues Fixed

1. Command Injection in PackageManager

Location: packages/core/packages/PackageManager.ts

Vulnerability: Package names were directly concatenated into shell commands, allowing potential command injection attacks.

Before:

public static addPackage(packageName: string, verbose: boolean = false): boolean {
    const command = `npm install ${packageName} --quiet --save`;
    Util.execSync(command, { stdio: "pipe", encoding: "utf8" });
}

public static async queuePackage(packageName: string, verbose = false) {
    const command = `npm install ${packageName} --quiet --no-save`;
    exec(command, {}, callback);
}

After:

public static addPackage(packageName: string, verbose: boolean = false): boolean {
    const args = ['install', packageName, '--quiet', '--save'];
    Util.spawnSync('npm', args, { stdio: "pipe", encoding: "utf8" });
}

public static async queuePackage(packageName: string, verbose = false) {
    const args = ['install', packageName, '--quiet', '--no-save'];
    const child = spawn('npm', args);
    // ... handle stdout/stderr/close events
}

2. Command Injection in Start Command

Location: packages/cli/lib/commands/start.ts

Vulnerability: Port parameter was directly interpolated into a command string without validation.

Before:

const execSyncNpmStart = (port: number, options: ExecSyncOptions): void => {
    if (port) {
        Util.execSync(`npm start -- --port=${port}`, options);
        return;
    }
    Util.execSync(`npm start`, options);
};

After:

const execSyncNpmStart = (port: number, options: ExecSyncOptions): void => {
    const args = ['start'];
    if (port) {
        // Validate port is a number to prevent command injection
        if (!Number.isInteger(port) || port < 0 || port > 65535) {
            Util.error(`Invalid port number: ${port}`, "red");
            return;
        }
        args.push('--', `--port=${port}`);
    }
    Util.spawnSync('npm', args, options);
};

Why This Approach is Secure

The fix uses spawn/spawnSync with argument arrays instead of string concatenation. This approach:

1. Prevents shell interpretation: Arguments are passed directly to the process without going through a shell
2. Eliminates injection vectors: Special characters like ;, |, &, $() in user input are treated as literal strings, not shell metacharacters
3. Adds validation: Port numbers are validated to be valid integers in the correct range (0-65535)

Testing

• Updated all affected unit tests to match the new secure implementation
• All existing tests continue to pass (298 passing, 8 pre-existing failures unrelated to these changes)
• Added proper mocking for spawn and spawnSync in test suites
• Verified optional chaining is properly transpiled to ES6-compatible code

Additional Changes

• Added getInstallArgs() helper method to centralize argument array creation
• Changed import from exec to spawn in PackageManager.ts
• Updated test expectations to verify secure command construction

Breaking Changes

None. The public API remains unchanged; only the internal implementation has been updated to be more secure.

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.